-
-
Notifications
You must be signed in to change notification settings - Fork 31
Fix eager loading relation for search operation #198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix eager loading relation for search operation #198
Conversation
WalkthroughThis PR normalizes a possibly-null include match result to an empty array in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test flow
participant Resp as Response::modelToResponse()
participant Includes as Request includes array
participant Curr as currentRequestArray
Test->>Resp: request with nested include
Resp->>Includes: find include match (first())
Includes-->>Resp: match or null
Resp->>Curr: normalize match with `?? []`
alt before: null returned
note right of Curr #ffdddd: null → potential null access
else after: normalized
note right of Curr #ddffdd: [] → safe array access
end
Resp->>Resp: read `selects`, `aggregates`, `gates`
Resp-->>Test: build response including eager-loaded nested relations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php (1)
584-645: Add test coverage for models without the eager-loaded relation.The test only creates one model with the full relation chain. However, the PR objective is to fix eager loading for search operations, and the AI summary mentions normalizing null results to empty arrays. Similar tests in this file (e.g.,
test_getting_a_list_of_resources_including_has_many_relationat lines 534-582) create two models—one with relations and one without—to verify both scenarios.Consider adding a second model without the relation to ensure the fix properly handles the case where
hasManyRelationWithEagerLoadingRelationreturns an empty result.Apply this diff to add coverage for the edge case:
public function test_getting_a_list_of_resources_including_has_many_relation_with_eager_loading_relations(): void { $matchingModel = ModelFactory::new() ->createOne()->fresh(); $hasMany = HasManyRelationFactory::new() ->for($matchingModel) ->createOne(); HasManyThroughRelationFactory::new() ->for($hasMany) ->createOne(); + + $matchingModel2 = ModelFactory::new()->createOne()->fresh(); Gate::policy(Model::class, GreenPolicy::class); Gate::policy(HasManyRelation::class, GreenPolicy::class); Gate::policy(HasManyThroughRelation::class, GreenPolicy::class); $response = $this->post( '/api/models/search', [ 'search' => [ 'includes' => [ [ 'relation' => 'hasManyRelationWithEagerLoadingRelation', ], ], ], ], ['Accept' => 'application/json'] ); $this->assertResourcePaginated( $response, - [$matchingModel], + [$matchingModel, $matchingModel2], new ModelResource(), [ [ 'has_many_relation_with_eager_loading_relation' => $matchingModel->hasManyRelationWithEagerLoadingRelation() ->orderBy('id') ->get() ->map(function ($relation) { $relation->has_many_through_relation = $relation->hasManyThroughRelation ->map(function ($relation) { $relation->has_many_relation = $relation->hasManyRelation->only( (new HasManyResource())->getFields(app()->make(RestRequest::class)) ); return $relation->only(array_merge( (new HasManyThroughResource())->getFields(app()->make(RestRequest::class)), ['has_many_relation'] )); })->toArray(); return $relation->only(array_merge( (new HasManyResource())->getFields(app()->make(RestRequest::class)), ['has_many_through_relation'] )); })->toArray(), ], + [ + 'has_many_relation_with_eager_loading_relation' => [], + ], ] ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Http/Response.php(1 hunks)tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php(3 hunks)tests/Support/Rest/Resources/HasManyThroughResource.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Support/Rest/Resources/HasManyThroughResource.php
🧰 Additional context used
📓 Path-based instructions (4)
{src,tests}/**/*.php
📄 CodeRabbit inference engine (composer.json)
All PHP code must be compatible with PHP 8.2 or higher
Files:
src/Http/Response.phptests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php
src/**/*.php
📄 CodeRabbit inference engine (composer.json)
src/**/*.php: Code should target Laravel framework v11–12 APIs only
Production classes must follow PSR-4: namespace Lomkit\Rest\ mapped to src/ with file paths matching class namespaces
Files:
src/Http/Response.php
tests/**/*.php
📄 CodeRabbit inference engine (composer.json)
tests/**/*.php: Test classes must follow PSR-4: namespace Lomkit\Rest\Tests\ mapped to tests/ with file paths matching class namespaces
Package tests should extend Orchestra\Testbench for Laravel package testing
Write tests compatible with PHPUnit 10/11 conventions
Files:
tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php
tests/Feature/**/*Test.php
📄 CodeRabbit inference engine (phpunit.xml)
Feature tests must reside under tests/Feature and be named with the suffix Test.php
Files:
tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php
🧬 Code graph analysis (1)
tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php (8)
tests/Support/Database/Factories/HasManyThroughRelationFactory.php (1)
HasManyThroughRelationFactory(8-28)tests/Support/Models/HasManyRelation.php (2)
HasManyRelation(7-18)hasManyThroughRelation(14-17)tests/Support/Models/HasManyThroughRelation.php (2)
HasManyThroughRelation(7-13)hasManyRelation(9-12)tests/Support/Rest/Resources/HasManyThroughResource.php (1)
HasManyThroughResource(11-30)tests/Support/Models/Model.php (4)
Model(10-118)hasManyRelationWithEagerLoadingRelation(50-54)hasManyThroughRelation(61-64)hasManyRelation(45-48)tests/Support/Policies/GreenPolicy.php (1)
GreenPolicy(8-100)tests/Support/Traits/InteractsWithResource.php (1)
assertResourcePaginated(13-47)tests/Support/Rest/Resources/HasManyResource.php (1)
HasManyResource(12-39)
🔇 Additional comments (2)
src/Http/Response.php (1)
81-84: LGTM! Critical fix for nested eager-loading.The addition of
?? []correctly handles the edge case wherefirst()returnsnullwhen no matching include is found. Without this, PHP 8.2+ would error when attempting to access array keys onnullat lines 91, 95, and 102. This ensures$currentRequestArrayis always an array, making the code safe for nested eager-loading scenarios where parent relations may not have explicit includes in the request.tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php (1)
13-13: LGTM!The new imports for
HasManyThroughRelationFactory,HasManyThroughRelation, andHasManyThroughResourceare correctly structured and follow PSR-4 conventions.Also applies to: 21-21, 30-30
|
Thanks, merging |
Summary by CodeRabbit
Bug Fixes
New Features